Skip to content

fix: parseStartAndEndTimes with times going over 12:00#330

Open
matt-franklin225 wants to merge 5 commits intomainfrom
parse-times-ending-hour-12
Open

fix: parseStartAndEndTimes with times going over 12:00#330
matt-franklin225 wants to merge 5 commits intomainfrom
parse-times-ending-hour-12

Conversation

@matt-franklin225
Copy link
Contributor

Description

Modified the logic for calculating minutes since midnight for the start and end times of a provided time interval in packages/stdlib/src/time-utils.ts. The previous logic caused the returned time to be shifted 12 hours earlier for times that ended in hour 12, resulting in values that were 720 minutes too early (sometimes even into the negative). This issue is now fixed by more precisely comparing the start and end times and noting whether am or pm is specified.

Related Issue

This closes issue #316

Motivation and Context

This fixes a logic issue in parsing times that is relevant to the websoc and larc scrapers, removing errant responses.

How Has This Been Tested?

I tested this logic locally by testing it with a combination of input strings that worked prior and ones that gave errant results to confirm that it fixed the error without creating new ones.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code involves a change to the database schema.
  • My code requires a change to the documentation.

@matt-franklin225 matt-franklin225 marked this pull request as ready for review March 5, 2026 00:17
@HwijungK
Copy link
Collaborator

HwijungK commented Mar 5, 2026

Could we rename the pr to something along the lines of "fix: parseStartAndEndTimes with times going over 12:00".

Any end time over 12:00 is now inferred to go over noon instead of midnight, not just the times that end with 12:xx.

For example, "10:00-1:00" now goes from 600 to 780 (10:00 am to 1:00 pm) instead of -120 to 60

@matt-franklin225 matt-franklin225 changed the title fix: parseStartAndEndTimes with times ending in hour 12 fix: parseStartAndEndTimes with times going over 12:00 Mar 5, 2026
Copy link
Member

@laggycomputer laggycomputer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Times that end on hour 12 end in the hour after noon if and only if the "p" suffix is present. This means the current third example is wrong.
  • An example/test case with start before noon and end not before 13:00 would be good.

@HwijungK
Copy link
Collaborator

HwijungK commented Mar 7, 2026

  • Times that end on hour 12 end in the hour after noon if and only if the "p" suffix is present. This means the current third example is wrong.

Wait i'm confused. Are we assuming that a midnight course could exist. If so it would logically make sense that '11:00-1:00' would also go through midnight.

@laggycomputer
Copy link
Member

Wait i'm confused. Are we assuming that a midnight course could exist. If so it would logically make sense that '11:00-1:00' would also go through midnight.

I don't want to rule that out, because that would mean re-scraping Websoc with this fix and seeing whether it's subtly off. The format (particularly, the fact that p is used for meetings and pm is used for finals) is very consistent.

@matt-franklin225
Copy link
Contributor Author

I don't want to rule that out, because that would mean re-scraping Websoc with this fix and seeing whether it's subtly off. The format (particularly, the fact that p is used for meetings and pm is used for finals) is very consistent.

In that case, is the passing midnight situation something we should account for, and if so, how do we handle minutes after midnight? We could either return them as minutes since the most recent midnight or minutes since the starting midnight.

@HwijungK
Copy link
Collaborator

HwijungK commented Mar 9, 2026

is the passing midnight situation something we should account for, and if so, how do we handle minutes after midnight

My best guess is that if there every is a case like that, the date would reference the earlier day, so Mar 9, 11:00-1:00 would probably mean at 11:00 on Mar 9 and ending 1:00 on Mar 10. It would probably be more right to say that the ending time is after 24*60, then to have a negative start time

@matt-franklin225
Copy link
Contributor Author

My best guess is that if there every is a case like that, the date would reference the earlier day, so Mar 9, 11:00-1:00 would probably mean at 11:00 on Mar 9 and ending 1:00 on Mar 10. It would probably be more right to say that the ending time is after 24*60, then to have a negative start time

Lmk what you think of the new version

Copy link
Collaborator

@HwijungK HwijungK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is breaking for "normal" pm time intervals like 3:00-4:00p, where the end time returns an hour greater than 24 * 60, and creates 12+ hr intervals for times going over midnight b/c the starttime isn't being pushed along with the end time

@matt-franklin225
Copy link
Contributor Author

this is breaking for "normal" pm time intervals like 3:00-4:00p, where the end time returns an hour greater than 24 * 60, and creates 12+ hr intervals for times going over midnight b/c the starttime isn't being pushed along with the end time

Previous issue should be fixed now

@laggycomputer laggycomputer requested a review from HwijungK March 11, 2026 22:06
Copy link
Collaborator

@HwijungK HwijungK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍👍

Comment on lines +25 to +26
if (endTimeMinute.includes("p") && startTime <= endTime) startTime += 12 * 60;
if (endTimeMinute.includes("p") && startTime > endTime) endTime += 12 * 60;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make these the same if and add a really short comment showing what sort of case we're trying to account for in both cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restructured the if statements and added a couple comments

Copy link
Member

@laggycomputer laggycomputer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant to lean more toward examples on (currently) lines 26 and 30.
Line 30 as is is somewhat clear, but the comment on line 26 doesn't add much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parseStartAndEndTimes can't handle times ending in hour 12

3 participants